WIP: POC to use orchestrion-js for instrumentation#20900
Conversation
size-limit report 📦
|
|
Note: dependency warning stuff should be addressed when this is merged/released: apm-js-collab/code-transformer-bundler-plugins#2 |
b1b6ed6 to
9e8b070
Compare
9e8b070 to
26ccdf4
Compare
26ccdf4 to
c8be420
Compare
a38481b to
c095626
Compare
| return [bundlerMarkerPlugin(), ...codeTransformerArray]; | ||
| } | ||
|
|
||
| function bundlerMarkerPlugin(): UnknownPlugin { |
There was a problem hiding this comment.
None of this is required!
| @@ -0,0 +1,33 @@ | |||
| // EXPERIMENTAL — entry point for `node --require @sentry/node/orchestrion app.js`. | |||
There was a problem hiding this comment.
We probably don't need to care about supplying a hook that works with --require
There was a problem hiding this comment.
ah, you mean it is good enough if everybody uses --import, even if their app is cjs?
There was a problem hiding this comment.
Yeah, @isaacs did some testing came up with this ESM code that can register all the hooks:
https://github.com/apm-js-collab/tracing-hooks/#usage
--import and --require just tell Node which kind of file you are passing after the flag and we can hook everything from ESM.
There was a problem hiding this comment.
changed it to import only!
5d957bf to
12ac21c
Compare
|
I also added an e2e test app using the vite plugin, however this is failing, will need to wait on v0.2.0 of the plugins as this updates to the latest version of the code transformer, which we need here. |
7d6f1f7 to
3de78da
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b089fd9. Configure here.
isaacs
left a comment
There was a problem hiding this comment.
This is looking very good!
My only concern/suggestion is that it should really not be node-specific. Deno and Bun are both supported by orchestrion now, so it'd be good to move this logic into a shared location so they can all benefit from it as we add instrumentations.
I'm not sure if it's best to (a) land this first, and then refactor this and the other @sentry/core/server stuff into @sentry-internal/server-utils, or (b) if we should bite that bullet first and then move this implementation on top of it. If we go with (a), then the move would be to land this now, and do server-utils as a second step. If (b), then it'd be good to get that done, and port this on top of it.
jup, eventually this should go into a new server-utils package or similar I guess - we can move this up any time! |

This is a WIP POC trying out usage of orchestrion-js for node SDK instrumentation.
Honestly it seems pretty straightforward... Usage for this POC is:
And then
This will disable the otel instrumentation that is already converted to orchestrion (in this PR, only Mysql) and add the respective orchestrion-based integrations instead. The exact API here is WIP and really just geared towards experimentation, so could change, and it's easy to see how this would be easier in v11 with this being the default.
Some general benefits of this approach:
--importscript only registers the mappings for orchestrion, all actual code registering stuff etc. happens inSentry.init(). This makes a bunch of things easier...--import. This also works when deploying to e.g. cloudflare etc. as long as one of the bundler plugins is used.